-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEATURE ds-extended-errors] Make adapter error extendable and add more error types #3586
Conversation
This could probably be labeled as [BUGFIX release]. |
Strictly this is not a bug fix. I am not sure where is the line of what we back porting in 1.13 series. @bmac you have an opinion? |
I think we should treat it as a new feature and feature flag it for a future 2.x release. If we find after it is release that it would provide value and help with the transition for users on the 1.13 series we can always backport it in the future. |
@tchak What is the status of this pr? |
@tchak @bmac can I also ask what the status of this is? This is really required because For info, I do think that
The key word here being additional. There's no requirement for a server to send a response that does include error objects within the JSON body - a simple HTTP status response with no content is valid. I know you've said in #3491 that the original HTTP response should not leak into the application, but it seems to me that the status code is required information considering that there is no obligation under the spec for the server to return error objects in its response body. |
@tchak Do you mind rebasing this pr and wrapping it in a feature flag? |
700bc04
to
010d534
Compare
@bmac done |
@@ -906,7 +910,18 @@ var RESTAdapter = Adapter.extend(BuildURLMixin, { | |||
let errors = this.normalizeErrorResponse(status, headers, payload); | |||
let detailedMessage = this.generatedDetailedMessage(status, headers, payload, requestData); | |||
|
|||
return new AdapterError(errors, detailedMessage); | |||
switch (status) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be wrapped in a isEnabled('ds-extended-errors');
?
Currently the only thing blocking this feature flag from being enabled is a lack of documentation. We need some more fleshed out API docs for all of the errors and a pr on the guides repo explaining the new errors. I haven't had any time to add docs for this feature but @lindyhopchris if you are up for the challenge a pr updating the error's doc blocks to describe when they trigger (and their status codes) would be a great place to start. |
Related: #3585 |
@bmac happy to give it a go, will probably have some time later this week to look at it and submit a PR. |
I've noted is that
[1] https://github.com/ember-cli/ember-ajax/blob/master/addon/errors.js#L194 EDIT: Either my brain was clouded when I wrote this, or the code has changed since. Regardless, there is already support for AbortError, TimeoutError and InvalidError (422). All awesome things! Extended errors is a great addition to Ember Data! ⛵️ |
re: your second point, and are used here: |
@lindyhopchris Thanks, you are right! Thanks for correcting me + this is great news! 😄 |
ping @bmac would be great to see this feature taken out from behind the feature flag. any update on when this might happen? |
This is going to help with questions like #3493 or #3491